Add env variable option for experimental #1138

seemethere · 2018-06-21T22:18:54Z

Signed-off-by: Eli Uriegas [email protected]

- What I did
Added an option to enable the experimental cli options with an environment variable

- How to verify it

Proof of work

~/go/src/github.com/docker/cli env_experimental eli@castlerock
❯ DOCKER_CLI_EXPERIMENTAL=enabled ./build/docker manifest

Usage:  docker manifest COMMAND

Manage Docker image manifests and manifest lists

Options:
      --orchestrator string   Orchestrator to use (swarm|kubernetes|all)

Commands:
  annotate    Add additional information to a local image manifest
  create      Create a local manifest list for annotating and pushing to a registry
  inspect     Display an image manifest, or manifest list
  push        Push a manifest list to a repository

Run 'docker manifest COMMAND --help' for more information on a command.

~/go/src/github.com/docker/cli env_experimental eli@castlerock
❯ DOCKER_CLI_EXPERIMENTAL=disabled ./build/docker manifest
docker manifest is only supported on a Docker cli with experimental cli features enabled

~/go/src/github.com/docker/cli env_experimental eli@castlerock
❯ ./build/docker manifest
docker manifest is only supported on a Docker cli with experimental cli features enabled

- Description for the changelog

* Added the option to enable experimental cli features through the `DOCKER_CLI_EXPERIMENTAL environment variable

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2018-06-22T04:57:11Z

cli/command/cli.go

@@ -180,6 +180,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {
 }

 func isEnabled(value string) (bool, error) {
+	// Environment variable always overrides configuration
+	if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" {


Can you update the documentation, and add this env-var to the list in https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#environment-variables ?

So this means that DOCKER_CLI_EXPERIMENTAL=false docker manifest will be valid? Maybe we should match the configuration file values instead of just checking the existence of the environment variable?

This does not seem very consistent at the moment, for instance there is

func contentTrustEnabled() bool { if e := os.Getenv("DOCKER_CONTENT_TRUST"); e != "" { if t, err := strconv.ParseBool(e); t || err != nil { // treat any other value as true return true } } return false }

but

func hide(cmd *cobra.Command) *cobra.Command { // If the environment variable with name "DOCKER_HIDE_LEGACY_COMMANDS" is not empty, // these legacy commands (such as `docker ps`, `docker exec`, etc) // will not be shown in output console. if os.Getenv("DOCKER_HIDE_LEGACY_COMMANDS") == "" { return cmd } …

Oh right...

Yeah for it to be uniform with what the value in the config is, it would need to be either enabled or disabled. Should I change it to that?

Here's what the diff would look like for that:

diff --git a/cli/command/cli.go b/cli/command/cli.go index 37e3e10e..0597341d 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -182,7 +182,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { func isEnabled(value string) (bool, error) { // Environment variable always overrides configuration if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" { - return true, nil + value = os.Getenv("DOCKER_CLI_EXPERIMENTAL") } switch value { case "enabled":

silvin-lubecki

Thank you for this PR @seemethere ! At first we wanted the user to explicitly enable the experimental features only by adding a value in the config file. But maybe with env var it is also explicit enough? WDYT @vdemeester ?

silvin-lubecki · 2018-06-22T09:02:22Z

cli/command/cli.go

@@ -180,6 +180,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {
 }

 func isEnabled(value string) (bool, error) {
+	// Environment variable always overrides configuration
+	if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" {


So this means that DOCKER_CLI_EXPERIMENTAL=false docker manifest will be valid? Maybe we should match the configuration file values instead of just checking the existence of the environment variable?

Signed-off-by: Eli Uriegas <[email protected]>

seemethere · 2018-06-24T22:38:11Z

Updated to environment variable to only accept enabled or disabled per @silvin-lubecki's suggestion, and added a mention in the docs per @thaJeztah's suggestion.

silvin-lubecki

Thank you @seemethere , LGTM !

vdemeester

LGTM 🐸

thaJeztah

LGTM

GordonTheTurtle added the status/0-triage label Jun 21, 2018

tiborvass added status/1-design-review and removed status/0-triage labels Jun 21, 2018

mat007 approved these changes Jun 22, 2018

View reviewed changes

thaJeztah requested changes Jun 22, 2018

View reviewed changes

silvin-lubecki requested changes Jun 22, 2018

View reviewed changes

seemethere force-pushed the env_experimental branch 2 times, most recently from 7326275 to 5c88f6f Compare June 24, 2018 21:03

seemethere requested a review from vdemeester as a code owner June 24, 2018 21:03

seemethere force-pushed the env_experimental branch from 5c88f6f to b86a3a9 Compare June 24, 2018 21:05

Add env variable option for experimental

e3bb62e

Signed-off-by: Eli Uriegas <[email protected]>

seemethere force-pushed the env_experimental branch from b86a3a9 to e3bb62e Compare June 24, 2018 21:15

silvin-lubecki approved these changes Jun 25, 2018

View reviewed changes

vdemeester approved these changes Jun 25, 2018

View reviewed changes

thaJeztah approved these changes Jun 25, 2018

View reviewed changes

thaJeztah merged commit 61e53fc into docker:master Jun 25, 2018

GordonTheTurtle added this to the 18.06.0 milestone Jun 25, 2018

seemethere deleted the env_experimental branch July 13, 2018 17:51

dims mentioned this pull request Sep 5, 2018

Support for manifests using docker experimental cli commands kubernetes/release#622

Merged

thaJeztah added the kind/experimental label Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add env variable option for experimental #1138

Add env variable option for experimental #1138

seemethere commented Jun 21, 2018 •

edited

Loading

thaJeztah Jun 22, 2018

silvin-lubecki Jun 22, 2018

mat007 Jun 22, 2018

silvin-lubecki Jun 22, 2018

seemethere Jun 22, 2018

seemethere Jun 22, 2018 •

edited

Loading

silvin-lubecki left a comment •

edited

Loading

silvin-lubecki Jun 22, 2018

seemethere commented Jun 24, 2018 •

edited

Loading

silvin-lubecki left a comment

vdemeester left a comment

thaJeztah left a comment

Add env variable option for experimental #1138

Add env variable option for experimental #1138

Conversation

seemethere commented Jun 21, 2018 • edited Loading

thaJeztah Jun 22, 2018

Choose a reason for hiding this comment

silvin-lubecki Jun 22, 2018

Choose a reason for hiding this comment

mat007 Jun 22, 2018

Choose a reason for hiding this comment

silvin-lubecki Jun 22, 2018

Choose a reason for hiding this comment

seemethere Jun 22, 2018

Choose a reason for hiding this comment

seemethere Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

silvin-lubecki left a comment • edited Loading

Choose a reason for hiding this comment

silvin-lubecki Jun 22, 2018

Choose a reason for hiding this comment

seemethere commented Jun 24, 2018 • edited Loading

silvin-lubecki left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

seemethere commented Jun 21, 2018 •

edited

Loading

seemethere Jun 22, 2018 •

edited

Loading

silvin-lubecki left a comment •

edited

Loading

seemethere commented Jun 24, 2018 •

edited

Loading